HYPERFLEET-538 - feat: CEL-based condition mapping engine - #329
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds CEL condition mappings to entity descriptors. Registry validation checks condition types and CEL expressions. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Registry
participant ResourceService
participant Adapter
participant ConditionMapper
participant ResourceStore
Registry->>ResourceService: Provide validated condition mappings
ResourceService->>ConditionMapper: Compile mappings by resource kind
Adapter->>ResourceService: Submit adapter statuses and data
ResourceService->>ConditionMapper: Apply mappings with resource and prior conditions
ConditionMapper-->>ResourceService: Return mapped conditions or error
ResourceService->>ResourceStore: Persist mapped conditions
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 5342 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/container | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
pkg/services/condition_mapper.go (2)
254-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the condition-type length check to startup validation.
rule.conditionTypecomes from configuration and never changes afterNewConditionMapper. Checking it on every evaluation, then failing the transaction, converts a static config defect into a permanent runtime rollback loop.pkg/registry/conditions.goalready validates rules at load time. EnforceMaxConditionTypeLengththere and drop the check here.Also note the log level: this path returns an error that rolls back the transaction, so
Warnunderstates it (HYG-02).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper.go` around lines 254 - 262, Move the MaxConditionTypeLength validation from the condition-evaluation path in the condition mapper into the startup rule validation in pkg/registry/conditions.go, alongside the existing rule checks. Remove the per-evaluation length check, warning, and error from the mapper so valid startup-validated rules proceed without runtime rejection.Source: Path instructions
443-479: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSingle-entry cache degrades to pure overhead under concurrent multi-resource load.
One
ConditionMapperinstance serves every resource of a kind. With N concurrent resources, eachApplymisses, performs the full marshal plusMaskSensitiveFields, then takes the write lock to evict the previous entry. The result is the uncached cost plus lock contention on the hot path, inside theGetForUpdaterow lock.Consider a small sharded or LRU cache keyed by resource ID, or drop the cache and keep the code simpler. The existing comment at lines 80-81 already anticipates this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper.go` around lines 443 - 479, Replace the single-entry cachedResource logic in ConditionMapper.getCachedOrBuildResource with a cache that retains multiple resources by ID, such as a small bounded LRU or sharded cache, so concurrent resources do not continually evict one another; alternatively remove the cache entirely if that matches the existing design guidance. Preserve generation-based invalidation and thread-safe access, while keeping the non-Resource fallback unchanged.pkg/services/condition_mapper_test.go (1)
1688-1754: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cache-invalidation test for the generation bump.
This test uses a distinct ID and generation per goroutine, so every call misses the cache. Nothing here proves the correctness claim documented at
condition_mapper.golines 72-78: that a generation bump invalidates the cached masked map. A stale hit would leak pre-PATCH spec values into mapped condition messages.Add a sequential test that calls
Applytwice with the same resource ID, mutates a spec field, bumpsGeneration, and asserts the mapped message reflects the new value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper_test.go` around lines 1688 - 1754, Add a sequential cache-invalidation test alongside TestConditionMapper_ConcurrentApply that uses the same resource ID, calls mapper.Apply once, mutates a spec field, increments Generation, and calls Apply again. Assert the second mapped condition message contains the updated spec value, proving the cached masked map is invalidated on a generation bump.Source: Path instructions
pkg/registry/conditions.go (1)
54-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the misleading comment and hoist the per-entity rebuild.
The comment says "Build reserved types for this specific entity".
buildReservedConditionTypesiterates every entity and derives adapter types from all of them, so the set is global, not per-entity. pkg/registry/registry.go line 173 calls this function once per entity, so both the reserved set and the CEL environment are rebuilt for every entity with mappings. The cost is startup-only, but the comment misstates the contract and invites a wrong change later.Correct the comment. Consider accepting a prebuilt reserved set and
*cel.Envfrom the caller soValidatebuilds each once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/conditions.go` around lines 54 - 61, Update the validation flow around Validate and buildReservedConditionTypes to describe the reserved types as global across all entities, not specific to one entity. Hoist construction of the reserved set and CEL environment to the registry caller so they are built once, then pass the prebuilt values into each per-entity validation call while preserving existing validation behavior.pkg/registry/conditions_test.go (1)
228-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated descriptor in the table cases.
Every case declares the same descriptor twice: once inside
entitiesand once asdescriptor. In all seven casesdescriptorequalsentities[0]. The duplication doubles the table length and lets the two copies drift, which would make a case pass for the wrong reason.Drop the
descriptorfield and passtt.entities[0]at line 498. Keep a separate field only if a case needs a descriptor that is not registered.Also add a case for an expression that compiles but returns the wrong type, once the type check from pkg/registry/conditions.go lines 170-202 lands.
As per path instructions: "Table-driven tests with t.Run() for repeated patterns".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/conditions_test.go` around lines 228 - 234, Remove the duplicated descriptor field from the table-driven test cases in the conditions test and update the invocation to pass tt.entities[0] instead; retain a separate descriptor only for cases where it differs from the registered entity. Add a t.Run() case covering an expression that compiles successfully but produces an incorrect type, exercising the type-checking behavior in the conditions implementation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/config.yaml.example`:
- Line 124: Update the CEL examples so s.adapter uses the bare adapter
identifiers reported by adapters and declared in required_adapters: change
landing-zone-adapter to the verified reported identifier in all affected
expressions, change validation-adapter to validation in
configs/config.yaml.example, and update all four corresponding expressions in
docs/config.md. In the documentation section, state that s.adapter must equal
the value reported by the adapter. Apply the changes at
configs/config.yaml.example lines 124-124 and docs/config.md lines 370-384.
- Around line 119-137: Comment out the example `conditions: []` key so
uncommenting the following CEL condition sequence does not create duplicate YAML
values; apply the same adjustment to the `NodePool` example near its
corresponding `conditions` entry. Preserve the existing example sequence and
indentation.
In `@pkg/registry/conditions.go`:
- Around line 170-202: Update validateCELExpression to retain the checked AST
returned by env.Check, then validate checked.OutputType() before env.Program:
require cel.BoolType for when expressions and cel.StringType for output fields,
while allowing cel.DynType when the result is not statically known. Use
expectedType.IsAssignableType(outputType), with the expected type selected from
condType, and preserve the existing parse, check, and compile error handling.
In `@pkg/services/condition_mapper.go`:
- Around line 24-27: Remove the unusable env CEL variable from the CEL
environment definition and stop binding emptyEnvMap during rule evaluation, or
otherwise make Check reject any expression referencing env before startup.
Ensure env references fail during validation rather than reaching evaluateRule
and triggering rollback retries.
In `@pkg/services/resource.go`:
- Around line 600-606: Update the recompute gate in the status-change logic
around hasMapper to also compare the persisted mapped conditions’ observed
generation with resource.Generation. Trigger recomputation when those
generations differ, while preserving the existing triggerAggregation and
Conditions/Data comparisons.
- Around line 53-71: Update buildConditionMappers to return an error when
NewConditionMapper fails instead of logging and continuing with a missing
mapper. Propagate that error through NewResourceService and make startup abort,
preserving successful mapper construction and registration for valid
descriptors.
In `@pkg/util/cel.go`:
- Around line 133-149: Update digFunc’s type traversal to support arbitrary
slice kinds, including []map[string]interface{}, by using reflection in addition
to the existing map and []interface{} handling. Preserve numeric index
validation and return types.NullValue for invalid, negative, or out-of-range
indices and unsupported values.
- Around line 87-99: The comments around limitedWriter and json.Encoder.Encode
incorrectly claim that encoding allocations are bounded. Remove or correct those
claims and update the related test description to reflect that limitedWriter
only limits bytes written, unless replacing the encoder with a genuinely
incremental serializer is within scope.
In `@pkg/util/mask_sensitive_test.go`:
- Around line 13-19: Replace every RegisterTestingT(t) call in the subtests of
TestMaskSensitiveFields and the other parallel top-level tests in this file with
a per-test Gomega assertion object created via NewWithT(t), and update
assertions to use that object while preserving existing test behavior.
---
Nitpick comments:
In `@pkg/registry/conditions_test.go`:
- Around line 228-234: Remove the duplicated descriptor field from the
table-driven test cases in the conditions test and update the invocation to pass
tt.entities[0] instead; retain a separate descriptor only for cases where it
differs from the registered entity. Add a t.Run() case covering an expression
that compiles successfully but produces an incorrect type, exercising the
type-checking behavior in the conditions implementation.
In `@pkg/registry/conditions.go`:
- Around line 54-61: Update the validation flow around Validate and
buildReservedConditionTypes to describe the reserved types as global across all
entities, not specific to one entity. Hoist construction of the reserved set and
CEL environment to the registry caller so they are built once, then pass the
prebuilt values into each per-entity validation call while preserving existing
validation behavior.
In `@pkg/services/condition_mapper_test.go`:
- Around line 1688-1754: Add a sequential cache-invalidation test alongside
TestConditionMapper_ConcurrentApply that uses the same resource ID, calls
mapper.Apply once, mutates a spec field, increments Generation, and calls Apply
again. Assert the second mapped condition message contains the updated spec
value, proving the cached masked map is invalidated on a generation bump.
In `@pkg/services/condition_mapper.go`:
- Around line 254-262: Move the MaxConditionTypeLength validation from the
condition-evaluation path in the condition mapper into the startup rule
validation in pkg/registry/conditions.go, alongside the existing rule checks.
Remove the per-evaluation length check, warning, and error from the mapper so
valid startup-validated rules proceed without runtime rejection.
- Around line 443-479: Replace the single-entry cachedResource logic in
ConditionMapper.getCachedOrBuildResource with a cache that retains multiple
resources by ID, such as a small bounded LRU or sharded cache, so concurrent
resources do not continually evict one another; alternatively remove the cache
entirely if that matches the existing design guidance. Preserve generation-based
invalidation and thread-safe access, while keeping the non-Resource fallback
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a5f5a2ce-e73b-4521-b92e-f45ecc97e5d4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (20)
configs/config.yaml.exampledocs/config.mdgo.modpkg/registry/conditions.gopkg/registry/conditions_test.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/condition_mapper.gopkg/services/condition_mapper_test.gopkg/services/resource.gopkg/services/resource_test.gopkg/util/cel.gopkg/util/cel_test.gopkg/util/mask_sensitive.gopkg/util/mask_sensitive_test.gopkg/util/naming.gopkg/util/naming_test.gotest/integration/condition_mapping_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- pkg/services/aggregation_test.go
Address code review findings from PR review: - Move condition validation from pkg/config to pkg/registry (better cohesion) - Add blank lines between adjacent top-level function declarations - Update config.yaml.example documentation for Unknown filtering behavior - Format code with gofmt Changes: - pkg/registry/conditions.go: Moved from pkg/config (validation logic belongs with registry) - pkg/registry/conditions_test.go: Moved from pkg/config - configs/config.yaml.example: Clarify Unknown filtering (entire adapter status dropped) - pkg/services/condition_mapper_test.go: Add blank lines between functions - pkg/util/cel_test.go: Add blank lines between functions - pkg/util/mask_sensitive_test.go: Add blank line before TestIsSensitiveKey All tests passing. No functional changes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Design doc (condition-mapping-design.md § Error Handling) mandates: "If a CEL expression fails, the entire mapping operation fails and the database transaction is rolled back." Changes: - Apply() signature: ([]api.ResourceCondition, error) instead of []api.ResourceCondition - CEL evaluation errors return error instead of skip-and-continue - resource.go propagates error as GeneralError → triggers rollback - Test: TestProcessAdapterStatus_ConditionMapperError_TriggersRollback Impact: CEL failures trigger 10s retry instead of 30min delay. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Without MarkForRollback, transaction commits despite error. Aligns with other error paths (lines 234, 265, 339, 932).
Expect(err) inside goroutine calls t.Fatal from non-test goroutine, which panics. Move assertion to main test goroutine instead. Fixes: send err through channel, assert on res.err after receive.
…ent skip validateFieldLengths error returned (nil, nil) - indistinguishable from when=false skip. Propagate error for consistency with CEL rollback-on-failure design (lines 170-177). Note: conditionType length validated at startup, so this is defense-in-depth (runtime path unreachable in practice).
When hasUnknown=true, adapterStatusToMapWithUnknownCheck allocated a full 4-key map that buildStatusesList immediately discarded. Return nil instead - caller guards with if !hasUnknown so never reads the map. Also adds test coverage for Unknown filtering path (missing coverage for hasUnknown=true code path).
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/services/condition_mapper_test.go (1)
1756-1780: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
testBuildActivationre-implements production logic, so the tests that use it do not test production code.
TestAdapterStatusToMapWithUnknownCheck_NilGuardandTestBuildActivation_NumericTypesConsistencyassert against this copy. IfbuildActivationWithCachechanges, for example if masking or a variable binding is added or removed, the helper keeps passing and the drift is not detected.Call
(&ConditionMapper{resourceKind: resourceKind}).buildActivationWithCache(ctx, statuses, resource)instead. The non-*api.Resourceinput already takes the uncached fallback path at Line 447, so no cache is involved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/condition_mapper_test.go` around lines 1756 - 1780, Remove the test-only testBuildActivation helper and update TestAdapterStatusToMapWithUnknownCheck_NilGuard and TestBuildActivation_NumericTypesConsistency to call (&ConditionMapper{resourceKind: resourceKind}).buildActivationWithCache(ctx, statuses, resource) directly. Preserve the existing inputs and assertions so these tests exercise the production activation-building path, including its uncached fallback for non-*api.Resource values.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/services/condition_mapper.go`:
- Around line 253-262: Move the conditionType length validation from the runtime
evaluation path into compileRule, returning a startup configuration error when
rule.conditionType exceeds registry.MaxConditionTypeLength. Remove the
corresponding check, warning log, and error return from the evaluation logic so
valid compiled rules are not revalidated on every evaluation.
---
Nitpick comments:
In `@pkg/services/condition_mapper_test.go`:
- Around line 1756-1780: Remove the test-only testBuildActivation helper and
update TestAdapterStatusToMapWithUnknownCheck_NilGuard and
TestBuildActivation_NumericTypesConsistency to call
(&ConditionMapper{resourceKind: resourceKind}).buildActivationWithCache(ctx,
statuses, resource) directly. Preserve the existing inputs and assertions so
these tests exercise the production activation-building path, including its
uncached fallback for non-*api.Resource values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0312380b-ab2a-41f4-b336-5464745689a5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (20)
configs/config.yaml.exampledocs/config.mdgo.modpkg/registry/conditions.gopkg/registry/conditions_test.gopkg/registry/descriptor.gopkg/registry/registry.gopkg/services/aggregation.gopkg/services/aggregation_test.gopkg/services/condition_mapper.gopkg/services/condition_mapper_test.gopkg/services/resource.gopkg/services/resource_test.gopkg/util/cel.gopkg/util/cel_test.gopkg/util/mask_sensitive.gopkg/util/mask_sensitive_test.gopkg/util/naming.gopkg/util/naming_test.gotest/integration/condition_mapping_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- pkg/services/aggregation_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- docs/config.md
- pkg/util/naming_test.go
- pkg/util/cel.go
- test/integration/condition_mapping_test.go
- pkg/services/resource_test.go
- pkg/registry/descriptor.go
- pkg/services/aggregation.go
- go.mod
- pkg/util/mask_sensitive.go
- pkg/util/cel_test.go
- pkg/registry/conditions_test.go
- pkg/util/naming.go
- pkg/util/mask_sensitive_test.go
- pkg/services/resource.go
- pkg/registry/registry.go
- pkg/registry/conditions.go
- configs/config.yaml.example
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/services/resource.go (1)
591-604: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRecompute mapped conditions after resource-generation changes.
The recomputation gate compares adapter
ConditionsandData, but it does not compareresource.Generation. A PATCH can changeresource.specorresource.Generationwhile the adapter report remains unchanged. The persisted mapped conditions then remain stale.Include
resource.Generationor the mapped conditions’ observed generation in the recomputation gate. This is the unresolved finding from the previous review.As per path instructions, cross-layer state and persistence contracts must be traced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/services/resource.go` around lines 591 - 604, The recomputation gate around triggerAggregation and hasMapper must also detect resource-generation changes, not only adapter status Conditions and Data changes. Compare resource.Generation with the mapped conditions’ observed generation (or the corresponding persisted generation field) so unchanged adapter reports still recompute after a spec-generation update, while preserving the existing duplicate-report gating behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hyperfleet-api/container/services.go`:
- Around line 9-19: Change ResourceService to return both the service and the
NewResourceService error instead of panicking when construction fails. Update
runServe to receive and handle that error before starting the API server,
preserving the startup failure flow without converting configuration errors into
a panic.
In `@pkg/services/resource.go`:
- Around line 44-45: Wrap the constructor error in the resource service creation
flow before returning it, adding service-level context that identifies the
failed operation while preserving the original cause for unwrapping. Replace the
bare return in the visible err-checking block and maintain the existing nil
result behavior.
- Around line 42-45: Update the test helpers calling NewResourceService to
handle its returned error instead of discarding it: assert the error is nil or
propagate it through the helper’s existing test/error mechanism. Apply this
consistently to all four helper call sites referenced in the comment so
mapper-construction failures are not masked.
---
Duplicate comments:
In `@pkg/services/resource.go`:
- Around line 591-604: The recomputation gate around triggerAggregation and
hasMapper must also detect resource-generation changes, not only adapter status
Conditions and Data changes. Compare resource.Generation with the mapped
conditions’ observed generation (or the corresponding persisted generation
field) so unchanged adapter reports still recompute after a spec-generation
update, while preserving the existing duplicate-report gating behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8733cf04-4dd5-4502-9077-428d3c5204a0
📒 Files selected for processing (14)
cmd/hyperfleet-api/container/services.godocs/config.mdgo.modpkg/registry/conditions.gopkg/registry/conditions_test.gopkg/registry/descriptor.gopkg/services/condition_mapper.gopkg/services/condition_mapper_test.gopkg/services/resource.gopkg/services/resource_test.gopkg/util/cel.gopkg/util/cel_test.gopkg/util/mask_sensitive_test.gopkg/util/naming_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (3)
- docs/config.md
- pkg/util/cel.go
- pkg/util/cel_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/util/naming_test.go
- pkg/registry/conditions.go
- go.mod
- pkg/services/condition_mapper_test.go
- pkg/services/condition_mapper.go
- pkg/registry/conditions_test.go
- pkg/services/resource_test.go
| svc, err := services.NewResourceService( | ||
| c.ResourceDao(), | ||
| c.ResourceLabelDao(), | ||
| c.AdapterStatusDao(), | ||
| c.ResourceConditionDao(), | ||
| c.GenericService(), | ||
| ) | ||
| if err != nil { | ||
| panic("failed to create resource service: " + err.Error()) | ||
| } | ||
| c.resourceService = svc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\bResourceService\s*\(\)' --glob '*.go' .
rg -n -C 8 '\b(New|Initialize|Run|Start|Serve|ListenAndServe)[A-Za-z0-9_]*\s*\(' --glob '*.go' cmdRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- services.go ---'
cat -n cmd/hyperfleet-api/container/services.go
printf '%s\n' '--- container definition and constructors ---'
rg -n -C 6 'type Container struct|func NewContainer|resourceService|NewResourceService' cmd/hyperfleet-api/container pkg
printf '%s\n' '--- serve startup sequence ---'
sed -n '40,190p' cmd/hyperfleet-api/servecmd/cmd.go
printf '%s\n' '--- relevant tests ---'
sed -n '1,100p' cmd/hyperfleet-api/container/container_test.go
printf '%s\n' '--- all ResourceService call sites, compact ---'
rg -n '\b[A-Za-z0-9_]*ResourceService\s*\(' --glob '*.go' .Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 33261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ResourceService constructor ---'
sed -n '1,115p' pkg/services/resource.go
printf '%s\n' '--- registry and CEL-related construction ---'
rg -n -C 8 'CEL|cel|condition|MustGet|NewResourceService' pkg cmd/hyperfleet-api --glob '*.go'
printf '%s\n' '--- startup call-order verifier ---'
python3 - <<'PY'
from pathlib import Path
source = Path("cmd/hyperfleet-api/servecmd/cmd.go").read_text()
resource = source.index("ctr.ResourceService()")
server = source.index("go apiServer.Start()")
print({
"resource_service_called_before_api_server_start": resource < server,
"resource_service_line": source[:resource].count("\n") + 1,
"api_server_start_line": source[:server].count("\n") + 1,
})
container = Path("cmd/hyperfleet-api/container/services.go").read_text()
print({
"constructor_error_panics": 'panic("failed to create resource service: " + err.Error())' in container,
"resource_service_assignment_is_unsynchronized": (
"if c.resourceService == nil" in container
and "c.resourceService = svc" in container
and "sync.Once" not in container
and "sync.Mutex" not in container
),
})
PYRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("cmd/hyperfleet-api/servecmd/cmd.go").read_text()
resource_pos = source.index("ctr.ResourceService()")
server_pos = source.index("go apiServer.Start()")
container = Path("cmd/hyperfleet-api/container/services.go").read_text()
print("resource_service_before_api_server_start:", resource_pos < server_pos)
print("resource_service_line:", source[:resource_pos].count("\n") + 1)
print("api_server_start_line:", source[:server_pos].count("\n") + 1)
print("constructor_error_panics:",
'panic("failed to create resource service: " + err.Error())' in container)
print("synchronization_primitive_present:",
any(token in container for token in ("sync.Once", "sync.Mutex", "sync.RWMutex")))
PY
printf '%s\n' '--- non-test ResourceService accessor call sites ---'
rg -n '\b[A-Za-z0-9_]*ResourceService\s*\(' --glob '*.go' --glob '!**/*_test.go' \
cmd pkg testRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 922
Propagate NewResourceService errors instead of panicking.
runServe already calls ResourceService() before starting the API server, so invalid CEL mappings fail during startup. Change the accessor to return the constructor error and handle it in runServe; do not convert configuration errors into panic (CWE-703).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hyperfleet-api/container/services.go` around lines 9 - 19, Change
ResourceService to return both the service and the NewResourceService error
instead of panicking when construction fails. Update runServe to receive and
handle that error before starting the API server, preserving the startup failure
flow without converting configuration errors into a panic.
Source: Path instructions
| ) (ResourceService, error) { | ||
| mappers, err := buildConditionMappers(registry.All()) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 '\bNewResourceService\s*\(' --glob '*.go' .Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 5075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all NewResourceService call sites ---'
rg -n -C 8 '\bNewResourceService\s*\(' --glob '*.go' .
printf '%s\n' '--- constructor and container accessor ---'
sed -n '1,90p' pkg/services/resource.go
sed -n '1,80p' cmd/hyperfleet-api/container/services.go
printf '%s\n' '--- registry and mapper construction ---'
rg -n -C 8 'buildConditionMappers|ConditionMappers|registry\.All\(' pkg/services --glob '*.go'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 13294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
pattern = re.compile(r'\bNewResourceService\s*\(')
for path in Path('.').rglob('*.go'):
text = path.read_text()
for match in pattern.finditer(text):
line = text.count('\n', 0, match.start()) + 1
start = text.rfind('\n', 0, match.start()) + 1
prefix = text[start:match.start()].strip()
print(f'{path}:{line}: {prefix}')
PY
printf '%s\n' '--- mapper error paths and registry descriptors ---'
rg -n -C 6 'func NewConditionMapper|return .*err|Conditions:|Register|registry\.All' pkg --glob '*.go'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 50392
Handle constructor errors in test helpers. The helpers at pkg/services/resource_test.go:282, :293, :305, and :315 discard the NewResourceService error. Assert or propagate it to avoid masking mapper-construction failures (CWE-391).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/services/resource.go` around lines 42 - 45, Update the test helpers
calling NewResourceService to handle its returned error instead of discarding
it: assert the error is nil or propagate it through the helper’s existing
test/error mechanism. Apply this consistently to all four helper call sites
referenced in the comment so mapper-construction failures are not masked.
Source: Path instructions
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the constructor error before returning.
Line 45 returns err without service-level context. Return a wrapped error that identifies the failed operation.
Proposed fix
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("initialize resource service: %w", err)
}As per path instructions, ERR-01 to ERR-04 require checked and wrapped errors. Bare error returns are not allowed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| return nil, err | |
| if err != nil { | |
| return nil, fmt.Errorf("initialize resource service: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/services/resource.go` around lines 44 - 45, Wrap the constructor error in
the resource service creation flow before returning it, adding service-level
context that identifies the failed operation while preserving the original cause
for unwrapping. Replace the bare return in the visible err-checking block and
maintain the existing nil result behavior.
Source: Path instructions
|
|
||
| // Field length constraints | ||
| const ( | ||
| MaxConditionTypeLength = 100 |
There was a problem hiding this comment.
Warning
Blocking
Category: JIRA
The JIRA AC (HYPERFLEET-538) and docs/config.md line 356 both say the type field limit is 128 characters/bytes, but this constant is set to 100. The test at conditions_test.go:583 also asserts against 100. Either the constant should be 128 to match the spec, or the doc/JIRA should be updated to reflect 100.
| MaxConditionTypeLength = 100 | |
| MaxConditionTypeLength = 128 |
| if len(reasonStr) > registry.MaxConditionReasonLength { | ||
| validatedReason = truncateUTF8(reasonStr, registry.MaxConditionReasonLength) | ||
| logger.With(ctx, "resource_kind", m.resourceKind, "condition_type", rule.conditionType). | ||
| Info("Condition reason truncated to max length") | ||
| } |
There was a problem hiding this comment.
Warning
Blocking
Category: JIRA
The JIRA AC (HYPERFLEET-538) says: "reason (256 chars, skip condition)" — implying the entire condition should be omitted when reason exceeds 256 chars. But the code here truncates the reason (same as message) and still produces the condition. If the design intent changed from "skip" to "truncate", please update the JIRA AC to match. If "skip condition" is the correct behavior, this needs a code change to return nil from evaluateRule when reason exceeds 256 chars.
3 participants
Summary
Implements a CEL-based condition mapping engine that allows operators to declaratively expose adapter-specific conditions in the public API
status.conditionsarray via YAML configuration — no code changes required.This PR continues the work originally authored by @ldornele in #315, with review feedback from @mliptak0 addressed in the final commit.
What Changed
pkg/services/condition_mapper.go): compiles mapping rules at startup (fail-fast), evaluates on every adapter status update. Errors trigger transaction rollback for timely retry (10s vs 30min).pkg/registry/conditions.go): each entity definesconditions[]withwhen/outputCEL expressions. Reserved types (Reconciled,LastKnownReconciled, per-adapter synthesized) cannot be overridden.pkg/util/cel.go):toJson(value)for marshaling,dig(target, "dot.path")for safe nested navigation.pkg/util/mask_sensitive.go): adapter data fields matching sensitive patterns are redacted before CEL evaluation.docs/config.md): condition mapping reference moved from inline config comments to the config guide.Review feedback addressed (from #315)
HYPERFLEET-538) fromconfigs/config.yaml.exampledocs/config.mdpkg/config/loader.gobuildConditionMappers()helper fromNewResourceServiceconstructorTest plan
go vet ./pkg/util/... ./pkg/config/...passesgo test ./pkg/util/...passes (CEL functions, masking, naming)go test ./...) — blocked by pre-existing mock build errors incluster_mock.go/node_pool_mock.go(unrelated to this PR;api.Cluster/api.NodePooltypes undefined on this branch)HYPERFLEET_TEST_CONDITION_MAPPING=1Original author
@ldornele — full implementation across 18 commits. This PR supersedes #315.